Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cosmetic): cannot find m-r-10 class in superset.less #20276

Merged
merged 6 commits into from
Jun 17, 2022

Conversation

Renderz
Copy link
Contributor

@Renderz Renderz commented Jun 6, 2022

SUMMARY

Cannot find '.m-r-10' css class in CollectionTable component, seems missing in src/assets/stylesheets/superset.less

BTW, maybe we should check any other missing css class.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
3

After:
4

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: Fixes Cannot find .m-r-10 class #20275
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Jun 6, 2022

Codecov Report

Merging #20276 (fdbf0d7) into master (ead1040) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #20276      +/-   ##
==========================================
+ Coverage   66.69%   66.70%   +0.01%     
==========================================
  Files        1739     1739              
  Lines       65153    65139      -14     
  Branches     6903     6897       -6     
==========================================
+ Hits        43453    43454       +1     
+ Misses      19947    19932      -15     
  Partials     1753     1753              
Flag Coverage Δ
javascript 51.75% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...tend/src/components/Datasource/CollectionTable.tsx 80.86% <100.00%> (+0.33%) ⬆️
...end/src/components/Datasource/DatasourceEditor.jsx 65.61% <100.00%> (+0.41%) ⬆️
...gins/plugin-chart-echarts/src/Pie/controlPanel.tsx 28.57% <0.00%> (-4.77%) ⬇️
superset-frontend/src/components/Modal/Modal.tsx 88.05% <0.00%> (ø)
...frontend/src/SqlLab/components/SqlEditor/index.jsx 51.36% <0.00%> (ø)
.../src/views/CRUD/data/savedquery/SavedQueryList.tsx 63.30% <0.00%> (ø)
...c/views/CRUD/data/database/DatabaseModal/index.tsx 32.16% <0.00%> (ø)
...dashboard/components/SliceHeaderControls/index.tsx 64.28% <0.00%> (ø)
...qlLab/components/EstimateQueryCostButton/index.tsx 5.26% <0.00%> (ø)
...es/superset-ui-core/src/query/buildQueryContext.ts 100.00% <0.00%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ead1040...fdbf0d7. Read the comment docs.

@rusackas
Copy link
Member

While this fixes the missing class, and I appreciate the contribution to resolve that, there are a couple things that are holding me up from approving/merging.

  1. The .m-r-10 definition exists in SQL Lab's main.less file, so if we were to go global with it, we should remove that in favor of the one you're adding.

  2. It looks like this class is only actually used in a couple places. It'd probably be better to style those two components with Emotion rather than adding more LESS (which we're trying to whittle away at)

  3. To confuse things further... I kind of LIKE the layout of the Before picture better... everything is nicely right aligned. Maybe we should just get rid of this margin once and for all? It sounds like you were solving a code problem more than a cosmetic problem, but let me know if I'm misinterpreting.

@Renderz
Copy link
Contributor Author

Renderz commented Jun 13, 2022

While this fixes the missing class, and I appreciate the contribution to resolve that, there are a couple things that are holding me up from approving/merging.

  1. The .m-r-10 definition exists in SQL Lab's main.less file, so if we were to go global with it, we should remove that in favor of the one you're adding.
  2. It looks like this class is only actually used in a couple places. It'd probably be better to style those two components with Emotion rather than adding more LESS (which we're trying to whittle away at)
  3. To confuse things further... I kind of LIKE the layout of the Before picture better... everything is nicely right aligned. Maybe we should just get rid of this margin once and for all? It sounds like you were solving a code problem more than a cosmetic problem, but let me know if I'm misinterpreting.

I found this problem when I was trying to add another Button on the right side after the one showing in the picture, and there was no margin between these two Button which should be there because both Button had '.m-r-10' class. So I raised up this issue.

But just as you mentioned, it looks nicely right aligned so maybe all those buttons should be removed from '.m-r-10' class? or use '.m-l-10' instead.

And maybe it is another good way to globalize SQL Labs' less file.

@michael-s-molina
Copy link
Member

It looks like this class is only actually used in a couple places. It'd probably be better to style those two components with Emotion rather than adding more LESS (which we're trying to whittle away at)

I think this is the best solution.

I found this problem when I was trying to add another Button on the right side after the one showing in the picture, and there was no margin between these two Button which should be there because both Button had '.m-r-10' class.

That's a valid use case. We just need to fix it using Emotion instead of LESS.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can adjust the margins to our theme multiples (12px) 😉

Renderz and others added 2 commits June 17, 2022 09:11
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
@michael-s-molina
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@michael-s-molina Ephemeral environment spinning up at http://52.13.42.77:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the contribution!

@michael-s-molina michael-s-molina merged commit f6f93aa into apache:master Jun 17, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

michael-s-molina pushed a commit that referenced this pull request Jun 23, 2022
* fix(cosmetic): cannot find m-r-10 class in superset.less

* fix: remove .m-r-10 class and use emotion instead

* Update superset-frontend/src/components/Datasource/CollectionTable.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

* Update superset-frontend/src/components/Datasource/DatasourceEditor.jsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
(cherry picked from commit f6f93aa)
@mistercrunch mistercrunch added 🍒 2.0.0 🍒 2.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S v2.0 🍒 2.0.0 🍒 2.0.1 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot find .m-r-10 class
4 participants